Defend against a:link:focus in GOV.UK Template #609

36degrees · 2018-03-16T11:30:11Z

GOV.UK Template includes a specific a:link:focus selector designed to make unvisited links a slightly darker blue when focussed, so we need to override the text colour for that combination of selectors so that the links within the various components do not end up with dark blue text when focussed when that is not the desired behaviour.

https://trello.com/c/rD8KRQle/816-remove-blue-colour-on-unvisited-button-coming-from-govuk-template
https://trello.com/c/fOiHvIjh/817-remove-blue-colour-on-unvisited-back-link-coming-from-govuk-template
https://trello.com/c/N3sczGfq/818-remove-blue-colour-on-unvisited-muted-link-coming-from-govuk-template

GOV.UK Template includes a specific a:link:focus selector [1] designed to make unvisited links a slightly darker blue when focussed, so we need to override the text colour for that combination of selectors so that the links within the various components do not end up with dark blue text when focussed when that is not the desired behaviour. [1]: https://github.com/alphagov/govuk_template/blob/73cf50c02ae335159ae241387d2ddd61f50d1b23/source/assets/stylesheets/_accessibility.scss#L63-L66

GOV.UK Template includes a specific `a:link:focus` selector [1] designed to make unvisited links a slightly darker blue when focussed, so we need to override the text colour for that combination of selectors so that the links within the muted link variant do not end up with dark blue text when focussed.

alex-ju · 2018-03-16T12:14:47Z

I would like to add something to this. The GOV.UK template selector was meant to make links slightly darker when focused - regardless of them being visited or not - as it says in the comment above that line /* Make links slightly darker when focused to improve contrast. */ - but the implementation was wrong at that time - a a:link:focus selector being using instead of a a:focus, thinking that :link will only apply it to links (e.g. as).

I raised #325 months ago for this to be fixed, but it got lost in translation.

Defending the links from such situations (chaining pseudo-classes) is a good thing, but if we defend against :link:focus, why not defending against :link:focus:hover and all other pseudo-classes combinations? (e.g. creating a mixin to generate these combinations quoting @Nooshu)

My opinion is that the first action should be to fix govuk_template issue, and then figure out if, how and in what degree we defend components, especially when chaining pseudo-classes.

36degrees · 2018-03-16T12:28:46Z

The GOV.UK template selector was meant to make links slightly darker when focused - regardless of them being visited or not - as it says in the comment above that line /* Make links slightly darker when focused to improve contrast. */ - but the implementation was wrong at that time - a a:link:focus selector being using instead of a a:focus, thinking that :link will only apply it to links (e.g. as).

I think in this case the implementation is actually right and the comment is wrong (or, at least, not clear) – from Ed's comment here the intention was just to make the unvisited links darker – leaving the visited links purple which already meets colour contrast - that being the case :link:focus is actually what is required.

Defending the links from such situations (chaining pseudo-classes) is a good thing, but if we defend against 🔗focus, why not defending against 🔗focus:hover and all other pseudo-classes combinations?

Because this is a specific fix against an issue integrating Frontend into a service that uses GOV.UK Template, where these selectors are found. Other combinations (thankfully!) don't exist, and I wouldn't look to defend against those either.

My opinion is that the first action should be to fix govuk_template issue, and then figure out if, how and in what degree we defend components, especially when chaining pseudo-classes.

I don't think we can assume that those looking to move to GOV.UK Frontend will always be running the latest version of GOV.UK Template – some users might still be on the current version, in which case they would need Frontend to compensate accordingly (especially as it's something that's easy to miss in testing, as we've seen).

alex-ju · 2018-03-16T12:34:27Z

If the intention was just to make the unvisited links darker then my PR's not solving it, so I'll close it for now. The fact that other combinations don't exist, doesn't mean they won't be in anytime soon.. but got your point.

edwardhorsford · 2018-03-16T12:58:40Z

From memory, I was trying to specifically target unvisited links as these were the ones that failed contrast checks - not visited links.

NickColley · 2018-03-16T14:14:36Z

src/breadcrumbs/_breadcrumbs.scss

    &:link,
    &:visited,
    &:hover,
-    &:active {
+    &:active,


Could we break these out into their own sections and do something like

.class { &:link, &:visited, &:hover, &:active { color: $govuk-text-colour; } if ($with-govuk-template) { ... overrides here } }

This would mean if we wanted to do 'pure' builds we could.

I like this in principle, but not sure how it'll work in practise – especially for users that are not compiling the Sass themselves. I'm not sure what we'd do for the dist build, for example?

Also, I'm not sure that we have the tooling in place to have confidence in this when introducing yet more complexity into the builds. In the example above we'd need to set color: $govuk-text-colour; in two places, and I could e.g. foresee one being changed without the other which could lead to inconsistent and hard to reproduce problems.

Pushed a branch to show what this might look like in practise.

For general builds we would set these to on by default, as is the same as your current code.

Low-fi is just to make sure each selector that overrides for govuk-template has a comment next to it, which most of these do?

NickColley · 2018-03-16T14:20:45Z

src/back-link/_back-link.scss

@@ -22,10 +22,18 @@
    // Underline is provided by a bottom border
    text-decoration: none;

+    // Override link colour to use text colour
+    //
+    // GOV.UK Template includes a specific a:link:focus selector designed to


Instead of 'GOV.UK Template' should we use 'alphagov/govuk_template' which is less ambiguous for people in the future?

If we intend to deprecate GOV.UK Template there will be people in the future that don't even know what this is.

Allow Frontend to selectively output styles only if another ‘product’ (Template, Frontend Toolkit, Elements) is also identified as being used in the project.

Create a new ‘text style link’ macro for the breadcrumbs, back link and skip link components to use. Abstract other link styles into mixins for consistency. Update breadcrumb, back link and skip link to use the new mixins.

kr8n3r · 2018-03-26T15:37:03Z

this looks good. 👍
want to wait for this to be approved to include it in #617

alex-ju

I think this can go in as it is for now if there are no blockers from the other reviewers.

hannalaakso

Looks good to me 🙂

Two non-blocking thoughts:

It would be good if the useful code comments, especially from /globals/core/_links.scss, could at some point make their way into our docs as instructions on how to set up correctly styled links in your service.
Out of the remit of this PR as not introduced by it... but I wonder if error summary links should come with a different hover colour. It would be good to understand/document why they don't, maybe it's something from user research.

36degrees · 2018-03-28T08:47:42Z

It would be good if the useful code comments, especially from /globals/core/_links.scss, could at some point make their way into our docs as instructions on how to set up correctly styled links in your service.

Is https://govuk-design-system-production.cloudapps.digital/styles/typography/#links the sort of thing you're thinking of?

hannalaakso · 2018-03-28T08:58:43Z

Yes that's capturing some of it but maybe on a slightly higher level 👍 I'm thinking about adding something about the compatibility modes and how to use things like the mixins together with each other such as this comment from _links.scss:

If you use this mixin in a component you must also include the govuk-link-common mixin in order to get the focus state.

However I wonder if some of this is too low level for the Design System and should live in GOV.UK Frontend /docs (or include a reference to go look in _links.scss etc.), I'm thinking that not everyone will go rooting around the core scss files but they might look at /docs.

I might be making too many assumptions here and we should just take this to user research.

kr8n3r · 2018-03-28T10:42:16Z

changelog :)

NickColley

Feels like the abstraction makes it confusing but I think I understand what's going on.

Otherwise looks good 👍 🚢

36degrees · 2018-03-28T11:50:57Z

Which abstraction, sorry? govuk_compatibility?

NickColley · 2018-03-28T12:13:42Z

@36degrees the compatibility one makes sense, the other ones add more layers, we should merge this.

36degrees added 2 commits March 16, 2018 11:26

govuk-design-system-ci deployed to govuk-frontend-review-pr-609 March 16, 2018 11:30 View deployment

36degrees changed the title ~~Defend components from a:link:focus in GOV.UK Template~~ Defend against a:link:focus in GOV.UK Template Mar 16, 2018

NickColley reviewed Mar 16, 2018

View reviewed changes

36degrees added 7 commits March 21, 2018 12:46

fixup! Defend components from a:link:focus in GOV.UK Template

264028f

Add ‘compatibility mode’ settings and tool

e426868

Allow Frontend to selectively output styles only if another ‘product’ (Template, Frontend Toolkit, Elements) is also identified as being used in the project.

Update footer to use compatibility mixin

293fcaf

Update error summary to use compatibility mixin

f82730e

Use abstracted text style link mixins

eeff255

Create a new ‘text style link’ macro for the breadcrumbs, back link and skip link components to use. Abstract other link styles into mixins for consistency. Update breadcrumb, back link and skip link to use the new mixins.

Add example of text links to links page

5d8735e

Use alphagov/govuk_template consistently

74e5012

alex-ju approved these changes Mar 27, 2018

View reviewed changes

hannalaakso reviewed Mar 28, 2018

View reviewed changes

NickColley approved these changes Mar 28, 2018

View reviewed changes

36degrees merged commit 3ce9848 into master Mar 28, 2018

36degrees deleted the override-link-focus-in-govuk-template branch March 28, 2018 12:17

hannalaakso mentioned this pull request Apr 12, 2018

Release v0.0.27-alpha #648

Merged

owenatgov mentioned this pull request Aug 14, 2023

Figure out how we use feature flags in GOV.UK Frontend #4086

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defend against a:link:focus in GOV.UK Template #609

Defend against a:link:focus in GOV.UK Template #609

36degrees commented Mar 16, 2018

alex-ju commented Mar 16, 2018 •

edited

Loading

36degrees commented Mar 16, 2018 •

edited

Loading

alex-ju commented Mar 16, 2018 •

edited

Loading

edwardhorsford commented Mar 16, 2018

NickColley Mar 16, 2018 •

edited

Loading

36degrees Mar 21, 2018

36degrees Mar 21, 2018 •

edited

Loading

NickColley Mar 21, 2018

NickColley Mar 22, 2018

NickColley Mar 16, 2018

kr8n3r commented Mar 26, 2018

alex-ju left a comment

hannalaakso left a comment

36degrees commented Mar 28, 2018

hannalaakso commented Mar 28, 2018

kr8n3r commented Mar 28, 2018 •

edited

Loading

NickColley left a comment •

edited

Loading

36degrees commented Mar 28, 2018

NickColley commented Mar 28, 2018

Defend against a:link:focus in GOV.UK Template #609

Defend against a:link:focus in GOV.UK Template #609

Conversation

36degrees commented Mar 16, 2018

alex-ju commented Mar 16, 2018 • edited Loading

36degrees commented Mar 16, 2018 • edited Loading

alex-ju commented Mar 16, 2018 • edited Loading

edwardhorsford commented Mar 16, 2018

NickColley Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

36degrees Mar 21, 2018

Choose a reason for hiding this comment

36degrees Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

NickColley Mar 21, 2018

Choose a reason for hiding this comment

NickColley Mar 22, 2018

Choose a reason for hiding this comment

NickColley Mar 16, 2018

Choose a reason for hiding this comment

kr8n3r commented Mar 26, 2018

alex-ju left a comment

Choose a reason for hiding this comment

hannalaakso left a comment

Choose a reason for hiding this comment

36degrees commented Mar 28, 2018

hannalaakso commented Mar 28, 2018

kr8n3r commented Mar 28, 2018 • edited Loading

NickColley left a comment • edited Loading

Choose a reason for hiding this comment

36degrees commented Mar 28, 2018

NickColley commented Mar 28, 2018

alex-ju commented Mar 16, 2018 •

edited

Loading

36degrees commented Mar 16, 2018 •

edited

Loading

alex-ju commented Mar 16, 2018 •

edited

Loading

NickColley Mar 16, 2018 •

edited

Loading

36degrees Mar 21, 2018 •

edited

Loading

kr8n3r commented Mar 28, 2018 •

edited

Loading

NickColley left a comment •

edited

Loading